Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ts file for Macaulay2Web syntax highlighting #3463

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

pzinn
Copy link
Contributor

@pzinn pzinn commented Sep 8, 2024

f7dabde removed a line from macaulay2.js.in which was used by Macaulay2Web.
Since Macaulay2Web uses typescript anyway, a new file macaulay2.ts.in is created with the missing line put back, and a comment that it's used by Macaulay2Web.

@pzinn
Copy link
Contributor Author

pzinn commented Sep 8, 2024

for the record, I can't request a review.

@d-torrance
Copy link
Member

Should we keep the .js file around? And should we update the webpack config to build from the .ts file instead?

@pzinn
Copy link
Contributor Author

pzinn commented Sep 9, 2024

what is the use of the webpack file?
I envisioned the js file as a standalone file for people who just want to enable M2 syntax highlighting on their web page.

@d-torrance
Copy link
Member

It generates the minified prism.js that ships with the Style package: https://macaulay2.com/doc/Macaulay2/share/Macaulay2/Style/prism.js

@pzinn
Copy link
Contributor Author

pzinn commented Sep 9, 2024

I see. you could use the ts file instead, but I don't think that'd be useful...

@d-torrance
Copy link
Member

Why don't you think it would be useful? I think it would be nice to have just a single source file for the prism grammar that both Macaulay2Web and the Style package can use.

@pzinn
Copy link
Contributor Author

pzinn commented Sep 9, 2024

I see. I just meant that there's no obvious benefit to switching to typescript, but yes, it should work equally well.

@@ -0,0 +1,32 @@
import Prism from "prismjs";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include this line? It wasn't before, and it doesn't appear that other prism language definition files include it: https://github.com/PrismJS/prism/tree/master/components

@pzinn
Copy link
Contributor Author

pzinn commented Sep 9, 2024

the import is typescript. imports in typescript vs javascript are confusing because they evolved a lot, with lots of possible syntaxes, but all I can say is this import is needed for typescript to know the variable Prism, otherwise it won't transpile.

@mahrud
Copy link
Member

mahrud commented Sep 10, 2024

Seems a bit strange to have both a .ts file and a .js file for prism, I'd rather just have one, but if we must have two, then they should have the same symbols/variables defined.

@d-torrance
Copy link
Member

import isn't just TypeScript -- it's been supported in JavaScript modules since ES6.

I actually don't see anything TypeScript-specific in the new file -- it's just exactly the .js file with the import statement added and M2SYMBOLS restored. I think we should either add some types and make it TypeScript or just keep it a .js file. And if it is a .ts file, then I agree with Mahrud that we should probably drop the .js file from the repository. Maybe there could be a script in package.json to transpile it to JavaScript, which I suppose we could either commit to git or not. Or maybe we could distribute it via npm.

I agree that import Prism from 'prismjs' is necessary to actually do anything with this file, but I don't think it belongs in the file itself. For consistency with other Prism languages, I think it should just define the language, and a third file would import Prism and the language file:

import Prism from 'prismjs';
import 'path/to/macaulay2.js';
// maybe do some other stuff

@pzinn
Copy link
Contributor Author

pzinn commented Sep 10, 2024

I don't think that's how it works in typescript. macaulay2.js (or ts) uses the variable Prism from the module prismjs, so it has to be imported in that file.

@mahrud
Copy link
Member

mahrud commented Sep 10, 2024

I don't think that's how it works in typescript. macaulay2.js (or ts) uses the variable Prism from the module prismjs, so it has to be imported in that file.

I think Doug's point is that the user should have called import Prism before loading macaulay2.ts (or js), which is consistent with other Prism.js languages at least. Does this lead to errors/warnings in typescript? It's been a while since I've used it.

@d-torrance
Copy link
Member

Yeah, you're right, Paul -- it's a TypeScript thing. I removed the import line from prism-M2.ts in the Macaulay2Web repo, ran webpack, and got the following:

ERROR in /home/profzoom/src/macaulay2/Macaulay2Web/src/client/prism-M2.ts
./src/client/prism-M2.ts 7:0-5
[tsl] ERROR in /home/profzoom/src/macaulay2/Macaulay2Web/src/client/prism-M2.ts(7,1)
      TS2304: Cannot find name 'Prism'.

But if I rename prism-M2.ts -> prism-M2.js, webpack runs just fine.

So why not just keep it as a .js file, especially since it doesn't use anything specific to TypeScript? That way, Macaulay2Web, the Style package, and potentially other users that want to use prism for highlighting M2 code could use it.

@pzinn
Copy link
Contributor Author

pzinn commented Sep 11, 2024

are you sure this works in ordinary js? in other words, I've no idea how a browser handles an export command in a javascript file that's not called with a module type or imported from another file. These feature have changed too many times for me to follow.

@d-torrance
Copy link
Member

Oh yeah, that's a good point -- export gives an "unexpected token" error in the browser.

Despite the convenience of having them together in the same file, I'm almost wondering if the symbols should go in another file? They're not really related to syntax highlighting with Prism.

@pzinn
Copy link
Contributor Author

pzinn commented Sep 11, 2024

yes, let me think about that.

@pzinn pzinn changed the title Add ts file for prism syntax highlighting Add ts file for Macaulay2Web syntax highlighting Sep 12, 2024
@pzinn
Copy link
Contributor Author

pzinn commented Sep 12, 2024

OK. now there is a separate file for the symbols used by Macaulay2Web.

@d-torrance d-torrance merged commit 415df96 into Macaulay2:development Sep 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants